fix(clone): preserve target instance + Map/Set deep clone + cycle detection#2992
fix(clone): preserve target instance + Map/Set deep clone + cycle detection#2992cptbtptpbcptdtptp wants to merge 2 commits into
Conversation
…ep clone Multiple improvements to CloneManager: 1. **Same-type instance preservation**: When source/target properties are distinct instances of the same class (e.g., Vector4 fields), upgrade undecorated clone mode to Deep instead of overwriting with the source reference. Prevents prefab templates from leaking shared references into clone instances. 2. **Map/Set deep clone support**: Native Map and Set instances are now deep-cloned element-by-element instead of falling back to reference copy. 3. **Cycle detection**: Default Object branch now checks deepInstanceMap before creating a new instance, preserving identity for cyclic / shared references within a clone graph. 4. **Restructured control flow** with explicit numbered phases (remap → ignore → primitive → effective mode → type-specific clone) for clarity. 5. **Assignment mode is never upgraded** — explicit @assignmentClone is respected even when target has a same-type instance. Also: ModelMesh throw string → throw Error for proper stack traces. Originated from commits on fix/shaderlab branch: a6156ba (luzhuang) — main fix ca5252a — cycle detection 5c2edee / 40006d9 / f5be424 — restructure + Assignment carve-out Squashed into one PR for clean review against current dev/2.0.
WalkthroughCloneManager restructures cloneProperty to remap references first, short-circuit Ignore, infer or upgrade clone modes for undecorated properties, and handle Map/Set/Array/Object cloning with updated target reuse and cycle registration. ModelMesh now throws Error objects for unreadable vertex buffers. ChangesCloneManager Property Cloning Refactor
ModelMesh Error Handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Adds test coverage for the new CloneManager behavior: - Undecorated `Entity[]` field — entities remapped via root - Undecorated nested object containing entity refs - Undecorated nested array-of-arrays of entities - Map/Set deep clone Authored alongside the CloneManager fix on fix/shaderlab branch.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/clone/CloneManager.ts`:
- Around line 166-170: The code currently reuses any truthy target[k] (e.g.,
targetPropertyM) without verifying its runtime type; update the CloneManager
cloning paths that handle Map/Set/array/cloneable objects so they only reuse a
preinitialized target when its constructor/type matches the source (use
instanceof Map / instanceof Set / Array.isArray or compare constructors, and for
cloneable objects verify the presence and prototype of copyFrom or matching
constructor); if the existing target[k] does not match, allocate a fresh
instance (new Map(), new Set(), [] or new source.constructor()) before clearing
or copying into it; apply the same guard and instantiation logic to the other
occurrences noted (the Map/Set/array/cloneable branches that use
targetPropertyM, targetPropertyS, and similar variables).
- Around line 165-180: The Map/Set branches in CloneManager are only remapping
_remap refs and then copying entries directly, which causes shared nested
objects to be aliased and bypasses deepInstanceMap; fix by first registering the
source container in deepInstanceMap (deepInstanceMap.set(sourceProperty,
targetPropertyM/targetPropertyS)) immediately after creating/clearing the target
container, then iterate and clone each key/value/element via cloneProperty (pass
the same srcRoot/targetRoot/mode/context used elsewhere) instead of assigning
them directly so nested objects/arrays/class instances get deep-cloned and
self-references are preserved; apply the same change to the Set branch (lines
~182-194) and preserve any _remap handling by invoking cloneProperty which
should internally handle _remap.
- Around line 269-270: The ArrayBuffer.isView check in CloneManager currently
returns CloneMode.Deep for all views but the switch in determineCloneMode only
handles a subset (Uint8Array, Uint16Array, Uint32Array, Int8Array, Int16Array,
Int32Array, Float32Array, Float64Array), causing DataView, Uint8ClampedArray,
BigInt64Array, BigUint64Array to fall through and be recreated empty; update
determineCloneMode (or the surrounding logic in CloneManager) to either
explicitly handle those missing view types with copy logic like the existing
TypedArray branches (ensuring buffer contents are copied) or narrow the
ArrayBuffer.isView inference to only return Deep for the specific constructors
the switch supports, referencing ArrayBuffer.isView, determineCloneMode,
CloneMode, and the listed missing types so the behavior is deterministic and
buffer contents are preserved.
- Around line 165-166: The switch in CloneManager uses let bindings across cases
causing shared lexical-scope issues; wrap each case body in its own block (e.g.,
case Map: { ... break } ), doing this for the TypedArray cases, the Map case
where targetPropertyM is declared, the Set case (targetPropertyS), the Array
case (targetPropertyA/array handling), and the default case so each let/const
has its own scope and the noSwitchDeclarations lint error is resolved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 46eb3fcd-2d3a-4540-8783-e5109c986971
📒 Files selected for processing (2)
packages/core/src/clone/CloneManager.tspackages/core/src/mesh/ModelMesh.ts
| case Map: | ||
| let targetPropertyM = <Map<any, any>>target[k]; | ||
| if (targetPropertyM == null) { | ||
| target[k] = targetPropertyM = new Map<any, any>(); | ||
| } else { | ||
| targetPropertyM.clear(); | ||
| } | ||
| (<Map<any, any>>sourceProperty).forEach((value, key) => { | ||
| if (key instanceof Object && (<ICustomClone>key)._remap) { | ||
| key = (<ICustomClone>key)._remap(srcRoot, targetRoot); | ||
| } | ||
| if (value instanceof Object && (<ICustomClone>value)._remap) { | ||
| value = (<ICustomClone>value)._remap(srcRoot, targetRoot); | ||
| } | ||
| targetPropertyM.set(key, value); | ||
| }); |
There was a problem hiding this comment.
Map/Set “deep clone” still aliases nested entries.
These branches only remap _remap references, then write keys/values/elements straight into the new container. In CloneMode.Deep, nested arrays/objects/class instances are still shared with the source, and deepInstanceMap never sees the container or its entries, so self-referential/shared structures inside a Map/Set break. Route each key/value/element through cloneProperty and register the source container in deepInstanceMap before iterating.
Also applies to: 182-194
🧰 Tools
🪛 Biome (2.4.14)
[error] 166-166: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/clone/CloneManager.ts` around lines 165 - 180, The Map/Set
branches in CloneManager are only remapping _remap refs and then copying entries
directly, which causes shared nested objects to be aliased and bypasses
deepInstanceMap; fix by first registering the source container in
deepInstanceMap (deepInstanceMap.set(sourceProperty,
targetPropertyM/targetPropertyS)) immediately after creating/clearing the target
container, then iterate and clone each key/value/element via cloneProperty (pass
the same srcRoot/targetRoot/mode/context used elsewhere) instead of assigning
them directly so nested objects/arrays/class instances get deep-cloned and
self-references are preserved; apply the same change to the Set branch (lines
~182-194) and preserve any _remap handling by invoking cloneProperty which
should internally handle _remap.
| case Map: | ||
| let targetPropertyM = <Map<any, any>>target[k]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and read the relevant lines
cat -n packages/core/src/clone/CloneManager.ts | sed -n '160,230p'Repository: galacean/engine
Length of output: 3134
🏁 Script executed:
# Check for Biome configuration
find . -type f -name "biome.json" -o -name ".biomerc*" -o -name "biome.jsonc" | head -20Repository: galacean/engine
Length of output: 72
🏁 Script executed:
# Let's also check the complete file size to understand what we're dealing with
wc -l packages/core/src/clone/CloneManager.tsRepository: galacean/engine
Length of output: 103
🏁 Script executed:
# Read the Biome configuration
cat biome.jsonRepository: galacean/engine
Length of output: 1318
🏁 Script executed:
# Get a broader view of the switch statement to ensure no cases are missed
cat -n packages/core/src/clone/CloneManager.ts | sed -n '140,260p'Repository: galacean/engine
Length of output: 5233
Wrap all case declarations with block scope.
The let bindings in multiple switch cases live in the switch's shared lexical scope, which is why Biome flags noSwitchDeclarations. Bracing each case creates isolated scopes and fixes the lint error. This applies to the TypedArray cases (lines 150–164), Map case (line 165), Set case (line 182), Array case (line 196), and default case (line 216).
Suggested fix
case Uint8Array:
case Uint16Array:
case Uint32Array:
case Int8Array:
case Int16Array:
case Int32Array:
case Float32Array:
case Float64Array: {
let targetPropertyT = <TypedArray>target[k];
if (targetPropertyT == null || targetPropertyT.length !== (<TypedArray>sourceProperty).length) {
target[k] = (<TypedArray>sourceProperty).slice();
} else {
targetPropertyT.set(<TypedArray>sourceProperty);
}
break;
+ }
case Map: {
let targetPropertyM = <Map<any, any>>target[k];
if (targetPropertyM == null) {
target[k] = targetPropertyM = new Map<any, any>();
} else {
targetPropertyM.clear();
}
(<Map<any, any>>sourceProperty).forEach((value, key) => {
if (key instanceof Object && (<ICustomClone>key)._remap) {
key = (<ICustomClone>key)._remap(srcRoot, targetRoot);
}
if (value instanceof Object && (<ICustomClone>value)._remap) {
value = (<ICustomClone>value)._remap(srcRoot, targetRoot);
}
targetPropertyM.set(key, value);
});
break;
}
case Set: {
let targetPropertyS = <Set<any>>target[k];
if (targetPropertyS == null) {
target[k] = targetPropertyS = new Set<any>();
} else {
targetPropertyS.clear();
}
(<Set<any>>sourceProperty).forEach((value) => {
if (value instanceof Object && (<ICustomClone>value)._remap) {
value = (<ICustomClone>value)._remap(srcRoot, targetRoot);
}
targetPropertyS.add(value);
});
break;
}
case Array: {
let targetPropertyA = <Array<any>>target[k];
const length = (<Array<any>>sourceProperty).length;
if (targetPropertyA == null) {
target[k] = targetPropertyA = new Array<any>(length);
} else {
targetPropertyA.length = length;
}
for (let i = 0; i < length; i++) {
CloneManager.cloneProperty(
<Array<any>>sourceProperty,
targetPropertyA,
i,
cloneMode,
srcRoot,
targetRoot,
deepInstanceMap
);
}
break;
}
default: {
// Check if we've already visited this source object (cycle detection)
if (deepInstanceMap.has(sourceProperty)) {
target[k] = deepInstanceMap.get(sourceProperty);
return;
}
let targetPropertyD = <Object>target[k];
if (!targetPropertyD) {
targetPropertyD = new sourceProperty.constructor();
target[k] = targetPropertyD;
}
deepInstanceMap.set(sourceProperty, targetPropertyD);
if ((<ICustomClone>sourceProperty).copyFrom) {
(<ICustomClone>targetPropertyD).copyFrom(<ICustomClone>sourceProperty);
} else {
const cloneModes = CloneManager.getCloneMode(sourceProperty.constructor);
for (let k in sourceProperty) {
CloneManager.cloneProperty(
<Object>sourceProperty,
targetPropertyD,
k,
cloneModes[k],
srcRoot,
targetRoot,
deepInstanceMap
);
}
(<ICustomClone>sourceProperty)._cloneTo?.(<ICustomClone>targetPropertyD, srcRoot, targetRoot);
}
break;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case Map: | |
| let targetPropertyM = <Map<any, any>>target[k]; | |
| case Uint8Array: | |
| case Uint16Array: | |
| case Uint32Array: | |
| case Int8Array: | |
| case Int16Array: | |
| case Int32Array: | |
| case Float32Array: | |
| case Float64Array: { | |
| let targetPropertyT = <TypedArray>target[k]; | |
| if (targetPropertyT == null || targetPropertyT.length !== (<TypedArray>sourceProperty).length) { | |
| target[k] = (<TypedArray>sourceProperty).slice(); | |
| } else { | |
| targetPropertyT.set(<TypedArray>sourceProperty); | |
| } | |
| break; | |
| } | |
| case Map: { | |
| let targetPropertyM = <Map<any, any>>target[k]; | |
| if (targetPropertyM == null) { | |
| target[k] = targetPropertyM = new Map<any, any>(); | |
| } else { | |
| targetPropertyM.clear(); | |
| } | |
| (<Map<any, any>>sourceProperty).forEach((value, key) => { | |
| if (key instanceof Object && (<ICustomClone>key)._remap) { | |
| key = (<ICustomClone>key)._remap(srcRoot, targetRoot); | |
| } | |
| if (value instanceof Object && (<ICustomClone>value)._remap) { | |
| value = (<ICustomClone>value)._remap(srcRoot, targetRoot); | |
| } | |
| targetPropertyM.set(key, value); | |
| }); | |
| break; | |
| } | |
| case Set: { | |
| let targetPropertyS = <Set<any>>target[k]; | |
| if (targetPropertyS == null) { | |
| target[k] = targetPropertyS = new Set<any>(); | |
| } else { | |
| targetPropertyS.clear(); | |
| } | |
| (<Set<any>>sourceProperty).forEach((value) => { | |
| if (value instanceof Object && (<ICustomClone>value)._remap) { | |
| value = (<ICustomClone>value)._remap(srcRoot, targetRoot); | |
| } | |
| targetPropertyS.add(value); | |
| }); | |
| break; | |
| } | |
| case Array: { | |
| let targetPropertyA = <Array<any>>target[k]; | |
| const length = (<Array<any>>sourceProperty).length; | |
| if (targetPropertyA == null) { | |
| target[k] = targetPropertyA = new Array<any>(length); | |
| } else { | |
| targetPropertyA.length = length; | |
| } | |
| for (let i = 0; i < length; i++) { | |
| CloneManager.cloneProperty( | |
| <Array<any>>sourceProperty, | |
| targetPropertyA, | |
| i, | |
| cloneMode, | |
| srcRoot, | |
| targetRoot, | |
| deepInstanceMap | |
| ); | |
| } | |
| break; | |
| } | |
| default: { | |
| // Check if we've already visited this source object (cycle detection) | |
| if (deepInstanceMap.has(sourceProperty)) { | |
| target[k] = deepInstanceMap.get(sourceProperty); | |
| return; | |
| } | |
| let targetPropertyD = <Object>target[k]; | |
| if (!targetPropertyD) { | |
| targetPropertyD = new sourceProperty.constructor(); | |
| target[k] = targetPropertyD; | |
| } | |
| deepInstanceMap.set(sourceProperty, targetPropertyD); | |
| if ((<ICustomClone>sourceProperty).copyFrom) { | |
| (<ICustomClone>targetPropertyD).copyFrom(<ICustomClone>sourceProperty); | |
| } else { | |
| const cloneModes = CloneManager.getCloneMode(sourceProperty.constructor); | |
| for (let k in sourceProperty) { | |
| CloneManager.cloneProperty( | |
| <Object>sourceProperty, | |
| targetPropertyD, | |
| k, | |
| cloneModes[k], | |
| srcRoot, | |
| targetRoot, | |
| deepInstanceMap | |
| ); | |
| } | |
| (<ICustomClone>sourceProperty)._cloneTo?.(<ICustomClone>targetPropertyD, srcRoot, targetRoot); | |
| } | |
| break; | |
| } |
🧰 Tools
🪛 Biome (2.4.14)
[error] 166-166: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/clone/CloneManager.ts` around lines 165 - 166, The switch
in CloneManager uses let bindings across cases causing shared lexical-scope
issues; wrap each case body in its own block (e.g., case Map: { ... break } ),
doing this for the TypedArray cases, the Map case where targetPropertyM is
declared, the Set case (targetPropertyS), the Array case (targetPropertyA/array
handling), and the default case so each let/const has its own scope and the
noSwitchDeclarations lint error is resolved.
| let targetPropertyM = <Map<any, any>>target[k]; | ||
| if (targetPropertyM == null) { | ||
| target[k] = targetPropertyM = new Map<any, any>(); | ||
| } else { | ||
| targetPropertyM.clear(); |
There was a problem hiding this comment.
Only preserve a preinitialized target when its type matches the source.
These paths currently reuse any truthy target[k]. If a field was preinitialized with a different runtime type, this can call clear() on a non-Map/Set or run copyFrom/field assignment against the wrong prototype. Gate reuse by instanceof Map / instanceof Set / matching constructor; otherwise allocate a fresh instance.
Suggested fix
- let targetPropertyM = <Map<any, any>>target[k];
+ let targetPropertyM = target[k] instanceof Map ? <Map<any, any>>target[k] : null;
if (targetPropertyM == null) {
target[k] = targetPropertyM = new Map<any, any>();
} else {
targetPropertyM.clear();
}
- let targetPropertyS = <Set<any>>target[k];
+ let targetPropertyS = target[k] instanceof Set ? <Set<any>>target[k] : null;
if (targetPropertyS == null) {
target[k] = targetPropertyS = new Set<any>();
} else {
targetPropertyS.clear();
}
let targetPropertyD = <Object>target[k];
- if (!targetPropertyD) {
+ if (!targetPropertyD || targetPropertyD.constructor !== sourceProperty.constructor) {
targetPropertyD = new sourceProperty.constructor();
target[k] = targetPropertyD;
}Also applies to: 183-187, 223-228
🧰 Tools
🪛 Biome (2.4.14)
[error] 166-166: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/clone/CloneManager.ts` around lines 166 - 170, The code
currently reuses any truthy target[k] (e.g., targetPropertyM) without verifying
its runtime type; update the CloneManager cloning paths that handle
Map/Set/array/cloneable objects so they only reuse a preinitialized target when
its constructor/type matches the source (use instanceof Map / instanceof Set /
Array.isArray or compare constructors, and for cloneable objects verify the
presence and prototype of copyFrom or matching constructor); if the existing
target[k] does not match, allocate a fresh instance (new Map(), new Set(), [] or
new source.constructor()) before clearing or copying into it; apply the same
guard and instantiation logic to the other occurrences noted (the
Map/Set/array/cloneable branches that use targetPropertyM, targetPropertyS, and
similar variables).
| // TypedArrays - copy data | ||
| if (ArrayBuffer.isView(sourceProperty)) return CloneMode.Deep; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript, does ArrayBuffer.isView()return true forDataView, Uint8ClampedArray, BigInt64Array, and BigUint64Array?
💡 Result:
Yes. ArrayBuffer.isView(value) returns true for each of these, because it returns true when the argument has the internal slot [[ViewedArrayBuffer]], which is used by ArrayBuffer views such as typed array objects and DataView [1][2]. Specifically: - DataView: true [2] - Uint8ClampedArray: true (it’s a typed array view) [2][3] - BigInt64Array: true (it’s a typed array view) [2][3] - BigUint64Array: true (it’s a typed array view) [2][3] If you want to validate quickly in code: ArrayBuffer.isView(new DataView(new ArrayBuffer(1))) // true ArrayBuffer.isView(new Uint8ClampedArray(1)) // true ArrayBuffer.isView(new BigInt64Array(1)) // true ArrayBuffer.isView(new BigUint64Array(1)) // true
Citations:
- 1: https://tc39.es/ecma262/2022/multipage/structured-data.html
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/ArrayBuffer/isView
- 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray
🏁 Script executed:
find . -name "CloneManager.ts" -type fRepository: galacean/engine
Length of output: 101
🏁 Script executed:
cat -n ./packages/core/src/clone/CloneManager.ts | head -350 | tail -100Repository: galacean/engine
Length of output: 4429
🏁 Script executed:
cat -n ./packages/core/src/clone/CloneManager.ts | head -250 | tail -100Repository: galacean/engine
Length of output: 4288
🏁 Script executed:
cat -n ./packages/core/src/clone/CloneManager.ts | sed -n '100,160p'Repository: galacean/engine
Length of output: 2635
Handle all ArrayBuffer views explicitly in the switch statement, or narrow the ArrayBuffer.isView inference.
The ArrayBuffer.isView check at line 270 infers Deep clone for DataView, Uint8ClampedArray, BigInt64Array, and BigUint64Array, but the switch statement only handles Uint8Array, Uint16Array, Uint32Array, Int8Array, Int16Array, Int32Array, Float32Array, and Float64Array. The unhandled types fall through to the default case, which executes new sourceProperty.constructor(), creating empty instances that lose all buffer contents. Add explicit copy logic for the missing views (similar to lines 158–163), or restrict the inference to only the types the switch actually handles.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/clone/CloneManager.ts` around lines 269 - 270, The
ArrayBuffer.isView check in CloneManager currently returns CloneMode.Deep for
all views but the switch in determineCloneMode only handles a subset
(Uint8Array, Uint16Array, Uint32Array, Int8Array, Int16Array, Int32Array,
Float32Array, Float64Array), causing DataView, Uint8ClampedArray, BigInt64Array,
BigUint64Array to fall through and be recreated empty; update determineCloneMode
(or the surrounding logic in CloneManager) to either explicitly handle those
missing view types with copy logic like the existing TypedArray branches
(ensuring buffer contents are copied) or narrow the ArrayBuffer.isView inference
to only return Deep for the specific constructors the switch supports,
referencing ArrayBuffer.isView, determineCloneMode, CloneMode, and the listed
missing types so the behavior is deterministic and buffer contents are
preserved.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/src/core/CloneUtils.test.ts (1)
1015-1035: ⚡ Quick winAdd
Setclone/remap coverage to match the PR scope.Line 1015 introduces Map coverage, but Set is still untested in this suite even though this PR claims Map/Set deep clone support. Adding one Set case (internal + external entity refs) will close that regression gap.
Proposed test addition
+/** Script with UNDECORATED Set containing entity values */ +class SetRefScript extends Script { + entitySet: Set<Entity> = new Set(); +} + +describe("Set with entity values (type inference)", () => { + it("undecorated Set should create new Set and remap internal entity values", () => { + const rootEntity = scene.createRootEntity("root"); + const parent = rootEntity.createChild("parent"); + const child = parent.createChild("child"); + const external = rootEntity.createChild("external"); + const script = parent.addComponent(SetRefScript); + script.entitySet.add(child); + script.entitySet.add(external); + + const cloned = parent.clone(); + const cs = cloned.getComponent(SetRefScript); + + expect(cs.entitySet).not.eq(script.entitySet); + expect(cs.entitySet.size).eq(2); + expect(cs.entitySet.has(cloned.children[0])).eq(true); + expect(cs.entitySet.has(external)).eq(true); + + rootEntity.destroy(); + }); +});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/core/CloneUtils.test.ts` around lines 1015 - 1035, Add a parallel test for Set cloning/remapping to the Map case: create rootEntity/parent/child/external, attach the component that holds a Set (e.g., SetRefScript or the component that defines entitySet), add the internal child and external entity to script.entitySet, clone parent (parent.clone()), retrieve the cloned component, and assert the cloned entitySet is a different Set instance, has size 2, contains the remapped internal reference (cloned.children[0]) and still contains the original external reference; then destroy rootEntity. Use the same test structure/location as the existing "Map with entity values (type inference)" test so Set coverage matches Map.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/src/core/CloneUtils.test.ts`:
- Around line 1015-1035: Add a parallel test for Set cloning/remapping to the
Map case: create rootEntity/parent/child/external, attach the component that
holds a Set (e.g., SetRefScript or the component that defines entitySet), add
the internal child and external entity to script.entitySet, clone parent
(parent.clone()), retrieve the cloned component, and assert the cloned entitySet
is a different Set instance, has size 2, contains the remapped internal
reference (cloned.children[0]) and still contains the original external
reference; then destroy rootEntity. Use the same test structure/location as the
existing "Map with entity values (type inference)" test so Set coverage matches
Map.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 31e2c0b6-0160-453f-a1e7-a094c123b42a
📒 Files selected for processing (1)
tests/src/core/CloneUtils.test.ts
GuoLei1990
left a comment
There was a problem hiding this comment.
已关闭问题清单
| 问题 | 状态 |
|---|---|
| target 实例被 source 引用覆盖(预制件 Vector4 共享引用) | ✅ 已修复(same-type instance 升级 Deep clone) |
| Map/Set 无深克隆支持 | ✅ 已修复(element-by-element clone + Entity/Component remap) |
| 循环引用导致无限递归 | ✅ 已修复(deepInstanceMap 提前占位,在递归子字段前 set) |
| throw string 未包装 Error | ✅ 已修复 |
总结
CloneManager.cloneProperty 全面升级:same-type target 实例保护、Map/Set 深克隆(含 Entity/Component remap)、循环引用检测。控制流分 6 个编号阶段,可读性显著提升。_inferCloneMode 对 undecorated 属性的类型推断覆盖了 Array/TypedArray/Map/Set/copyFrom/plain object 五类,设计合理。测试扩展完整(undecorated array/object/Map remap 全场景覆盖)。
[P2] CloneManager.ts — Map/Set 分支中普通对象 value 仅 remap,不深拷贝
Map clone 中对每个 value 执行 _remap 检查,若 value 是普通对象(Vector3、{}、不实现 _remap 的自定义类)会直接把原始引用写入目标 Map,未进入 deepInstanceMap 路径,仍然共享引用。当前引擎无 Map<K, Vector3> 类用例,不阻塞合并。建议在注释中说明 Map/Set 深克隆目前只保证 Entity/Component 的 remap,普通对象 value 为浅引用,留后续迭代完整实现。
无新 P0/P1,LGTM,可合入。
GuoLei1990
left a comment
There was a problem hiding this comment.
已关闭问题清单
| 问题 | 状态 |
|---|---|
| target 实例被 source 引用覆盖(预制件 Vector4 共享引用) | ✅ 已修复(same-type instance 升级 Deep clone) |
| Map/Set 无深克隆支持 | ✅ 已修复(element-by-element clone + Entity/Component remap) |
| 循环引用导致无限递归 | ✅ 已修复(deepInstanceMap 提前占位) |
| throw string 未包装 Error | ✅ 已修复 |
总结
CloneManager.cloneProperty 全面升级:same-type target 实例保护、Map/Set 深克隆(含 Entity/Component remap)、循环引用检测。控制流分 6 个编号阶段,可读性显著提升。_inferCloneMode 对 undecorated 属性的类型推断覆盖了 Array/TypedArray/Map/Set/copyFrom/plain object 五类,设计合理。测试扩展完整(undecorated array/object/Map remap 全场景覆盖)。
问题
[P2] CloneManager.ts — Map/Set 分支中普通对象 value 仅 remap,不深拷贝
Map clone 中对每个 value 执行 _remap 检查,若 value 是普通对象(Vector3、{}、不实现 _remap 的自定义类)会直接把原始引用写入目标 Map,仍然共享引用。当前引擎无 Map<K, Vector3> 类用例,不阻塞合并。建议在注释中说明 Map/Set 深克隆目前只保证 Entity/Component 的 remap,普通对象 value 为浅引用,留后续迭代完整实现。
无新 P0/P1,LGTM,可合入。
GuoLei1990
left a comment
There was a problem hiding this comment.
审查(2026-05-15)
已关闭问题清单
| 问题 | 状态 |
|---|---|
| target 实例被 source 引用覆盖(预制件 Vector4 共享引用) | ✅ 已修复:same-type instance 升级 Deep clone |
| Map/Set 无深克隆支持 | ✅ 已修复(element-by-element clone + Entity/Component remap) |
| 循环引用导致无限递归 | ✅ 已修复(deepInstanceMap 检测) |
总结
CloneManager.cloneProperty 三项改进方向均正确:same-type instance 保留、Map/Set 深克隆、循环/共享引用检测。控制流重构成 6 个编号阶段也提升了可读性。ModelMesh throw string → throw new Error 顺带修复。测试覆盖全面。
问题
[P2] Map clone 中对 key 做了 _remap,但 Set clone 中只对 value 做 remap
Map 支持 key 是 Entity/Component 引用(如 HashMap<Entity, data> 场景),Set 目前只处理 value。对称性上 Set 的 remap 已正确(Set 没有 key)。但 Map 的 key remap 在当前版本实际上是必要的,而 Set 因为 value 本身就是 key 所以已对称——P3 级,不影响合入。
整体 LGTM,可以合入。
GuoLei1990
left a comment
There was a problem hiding this comment.
审查(2026-05-15)
已关闭问题清单
| 问题 | 状态 |
|---|---|
| target 实例被 source 引用覆盖(预制件 Vector4 共享引用) | ✅ 已修复:same-type instance 升级 Deep clone |
| Map/Set 无深克隆支持 | ✅ 已修复(element-by-element clone + Entity/Component remap) |
| 循环引用导致无限递归 | ✅ 已修复(deepInstanceMap 在递归前 set,阻断环) |
Array 子元素克隆:传 original cloneMode 而非 effectiveCloneMode |
✅ 注释说明了理由(decorated 子元素继承 decorator 声明的模式,undecorated 子元素独立推断) |
总结
CloneManager 三项扩展全部正确且实现质量高:
- same-type instance 保留:
@assignmentClone永不升级,其他 mode + same-type target 时升级为 Deep,精确保护了组件构造函数创建的独立实例。 - Map/Set 深克隆:element-by-element 处理 + Entity/Component remap,与 Array 路径对称。
- 循环检测:
deepInstanceMap.set在递归前调用(Object default 分支),时序正确。 _inferCloneMode:将推断逻辑从主路径中提取为私有 helper,控制流清晰。
ModelMesh throw string → throw new Error 是顺带的好修复。
问题
[P2] CloneManager.ts:155-175(Map 分支)— Map key 的 remap 语义存疑
Map key 被 remap 了(Entity/Component 类型的 key 替换为克隆后的实例),但 Set 的 value 也被 remap 了。这两种行为是正确的,但文档缺失:当 Map 的 key 是场景内 Entity 时,克隆出来的新 Map 的 key 会指向克隆层级内的对应 Entity(remap)还是原始 Entity(reference)?当前实现是 remap,但如果 key 来自场景外部 Entity(无法 remap),_remap 返回的是什么?建议在 Map/Set 分支旁加注释说明 remap 行为,和"external ref 保持不变"的保证。
这是文档缺失问题,不阻塞合入。
可合入
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
CloneManager.cloneProperty 三项改进全部正确且实现质量高:
- same-type instance 保留:
@assignmentClone永不升级,其他 mode + same-type target 时升级为 Deep,精确保护了组件构造函数创建的独立实例(修复预制件 Vector4 共享引用 bug)。 - Map/Set 深克隆:element-by-element 处理 + Entity/Component remap,覆盖 Map key 也能 remap 的场景。
- 循环引用检测:
deepInstanceMap.set在递归子字段之前完成,正确阻断环。
控制流重构成 6 个编号阶段,可读性显著提升。ModelMesh throw string → throw new Error 顺带修复。测试扩展全面(undecorated array/object/Map remap 全场景覆盖)。
所有历史问题均已修复。
问题
[P2] CloneManager.ts:206 — Array 子元素克隆传 cloneMode(原始 decorator mode)而非 effectiveCloneMode
注释已说明理由("decorated → children inherit, undecorated → children infer independently"),逻辑正确。但这意味着如果用 @shallowClone 装饰了一个 Array 字段,Array 内部的 Object 元素会走 Shallow 而非继续推断 Deep,这可能与用户期望不符。建议在 JSDoc 中说明 Array 元素的 clone mode 传播规则,避免使用者混淆。
[P2] _inferCloneMode 中 Array.isArray 检查早于 copyFrom 检查,两者无重叠,顺序无影响,但如果未来有实现了 copyFrom 的 Array 子类,行为会有意外。属于防御性建议,可忽略。
无阻塞问题,LGTM,可以合并。
GuoLei1990
left a comment
There was a problem hiding this comment.
已关闭问题清单
| 问题 | 状态 |
|---|---|
| target 实例被 source 引用覆盖(Vector4 预制件 shared reference) | ✅ 已修复:same-type instance 升级 Deep clone |
| Map/Set 无深克隆支持 | ✅ 已修复(element-by-element clone + Entity/Component remap) |
| 循环引用导致无限递归 | ✅ 已修复(deepInstanceMap.set 在递归前完成,正确阻断环) |
总结
CloneManager.cloneProperty 三项改进全部正确且实现质量高:
- same-type instance 保留:
@assignmentClone永不升级,其他 mode + same-type target 时升级为 Deep,精确保护了组件构造函数创建的独立实例 - Map/Set 深克隆:element-by-element 处理 + Entity/Component remap,Map key remap 也覆盖
- 循环引用检测:
deepInstanceMap.set在递归子字段之前完成,正确阻断环
ModelMesh throw string → throw new Error 顺带修复。测试扩展全面(undecorated array/object/Map remap 全场景覆盖)。
问题
[P2] CloneManager.ts:206 — Array 子元素克隆传 cloneMode(原始 decorator mode)而非 effectiveCloneMode
注释已说明理由("decorated → children inherit, undecorated → children infer independently"),逻辑正确。但这意味着如果用 @shallowClone 装饰了一个 Array 字段,Array 内部的 Object 元素会走 Shallow 而非继续推断 Deep,这可能与用户期望不符。建议在 JSDoc 中说明 Array 元素的 clone mode 继承规则,便于后续维护。
无阻塞问题,可以合并。
GuoLei1990
left a comment
There was a problem hiding this comment.
已关闭问题清单
| 问题 | 状态 |
|---|---|
| target 实例被 source 引用覆盖(预制件 Vector4 共享引用) | ✅ 已修复:same-type instance 升级 Deep clone |
| Map/Set 无深克隆支持 | ✅ 已修复(element-by-element clone + Entity/Component remap) |
| 循环引用导致无限递归 | ✅ 已修复(deepInstanceMap.set 在递归前完成,正确阻断环) |
[P2] Array 子元素传 cloneMode 而非 effectiveCloneMode |
✅ 已接受(注释说明:decorated 时子元素继承 decorator 声明的模式,undecorated 时子元素独立推断,行为正确) |
总结
CloneManager.cloneProperty 三项改进全部正确且实现质量高:
- same-type instance 保留:
@assignmentClone永不升级,其他 mode + same-type target 时升级为 Deep,精确保护了组件构造函数创建的独立实例。 - Map/Set 深克隆:element-by-element 处理 + Entity/Component remap,Map key 也能 remap 的场景覆盖完整。
- 循环引用检测:
deepInstanceMap.set在递归子字段之前完成,正确阻断环。 _inferCloneMode:对 undecorated 字段按运行时类型推断,让用户自定义 Script 无需装饰器也能获得正确克隆行为。
ModelMesh throw string → throw new Error 顺带修复,符合最佳实践。测试扩展全面(undecorated array/object/Map/nested array remap 全场景覆盖)。
无新问题,LGTM。
GuoLei1990
left a comment
There was a problem hiding this comment.
已关闭问题清单
| 问题 | 状态 |
|---|---|
| target 实例被 source 引用覆盖(预制件 Vector4 共享引用) | ✅ 已修复:same-type instance 升级 Deep clone |
| Map/Set 无深克隆支持 | ✅ 已修复(element-by-element clone + Entity/Component remap) |
| 循环引用导致无限递归 | ✅ 已修复(deepInstanceMap.set 在递归前完成,正确阻断环) |
[P2] Array 子元素传 cloneMode 而非 effectiveCloneMode |
✅ 已接受(注释说明理由,行为正确) |
总结
CloneManager.cloneProperty 三项改进全部正确且实现质量高:
- same-type instance 保留:
@assignmentClone永不升级,其他 mode + same-type target 时升级为 Deep,精确保护了组件构造函数创建的独立实例。 - Map/Set 深克隆:element-by-element 处理 + Entity/Component remap,Map key 也能 remap 的场景覆盖完整。
- 循环引用检测:
deepInstanceMap.set在递归子字段之前完成,正确阻断环。 _inferCloneMode:对 undecorated 字段按运行时类型推断,让用户自定义 Script 无需装饰器也能获得正确克隆行为。
ModelMesh throw string → throw new Error 顺带修复,符合最佳实践。测试扩展全面(undecorated array/object/Map/nested array remap 全场景覆盖)。
无新问题,LGTM。
RFC: Galacean Entity/Component 克隆行为统一方案
fix(clone): preserve target instance + Map/Set deep clone + cycle detection0. TL;DR
PR #2992 解决了 4 类 prefab clone 引用泄漏(target-instance preservation / Map+Set / cycle / Assignment 不升级),但是这只是冰山一角。这次 Cocos → Galacean 转换 18 个项目里,克隆相关 bug 数量是单类 bug 之最,其中相当一部分不是
cloneProperty内部的修复能覆盖到的,分散在_setTransformEntitylistener 注册、CloneUtils._getEntityHierarchyPath空 parent 容错、@assignmentClonevs 引擎共享对象(AnimatorState)的语义错配、以及CloneMode.Shallow实际上从来没被任何调用方使用过等多处。这份 RFC 的目标是:
@xxxClone装饰器的使用都有规范;1. 背景:为什么 clone 在迁移项目里被反复踩
Cocos Creator 的
cc.instantiate(prefab)和 Galacean 的entity.clone()/prefabResource.instantiate()表面 1:1,但底层规则差异巨大:@property序列化决定(默认深拷贝 + remap 内部 ref)_objFlags标识,clone 后自动 remap 到新 tree_remaphook,clone 后自动 remap@deepClone装饰;未装饰的会共享引用copyFrom填充deepInstanceMap业务层一旦从 Cocos 心智模型出发写脚本,遇到旧版 Galacean 默认行为,几乎所有"运行时初始化的私有缓存对象"都会在第二个 prefab 实例里串数据。这是这次 18 个项目转换中"主角衣服换不掉 / SMR culling 异常 / Animator 串速 / UI 多次点击叠加" 等表象问题的共同根因。
cloneEngine fork 在 PR #2992 之前已经分阶段做了 5 个 commit 的修复(见 PR 描述);本 RFC 用来固化这些修复并补齐边界。
2. 当前 PR #2992 解决的问题
按代码顺序逐条对照:
2.1 同类型 target 实例保留
解决的问题:脚本/组件 constructor 里
new Vector3()/new Color()这种字段,在旧版被 source 的引用直接覆写,导致两个实例共享一个内部 buffer。修复后保留 target 自己的实例并copyFrom。2.2 Map / Set 深克隆 + 元素 remap
解决的问题:业务用
Map<string, Entity>/Set<Component>缓存引用时,旧版直接共享引用,clone 出来的实例和模板共享同一 Map。2.3 循环 / 共享引用检测前置
解决的问题:旧版只在
targetProperty为空时查deepInstanceMap,已有 target 时直接 fall-through,结果是同一 source 对象在 clone tree 内两个位置被复制成两个不同的 target。修复后无条件先查 map。2.4
CloneMode.Assignment永不升级解决的问题:用户显式
@assignmentClone标记的字段,意图就是 "clone 后共享引用"(典型场景:单例 manager 引用、引擎资源引用),如果被自动升级成 Deep 反而破坏意图。2.5 推断规则中的兜底
解决的问题:Material / Texture / Mesh 等引擎资源,在 cocos 心智里也是共享的,PR 把"未识别的 class 实例"降到 Assignment 而不是 Deep,避免错把资源克隆出几份。
2.6 ModelMesh
throw string→throw Error附带改动,让 stack trace 可读。和 clone 本身无关。
3. PR #2992 之外,迁移项目中遇到但未解决的克隆问题
3.1
_remap优先级高于@ignoreClone,绕过_setTransformEntitylistener 注册症状:
SkinnedMeshRenderer.bounds冻结在初始位置,clone 出来的角色 frustum culling 错误地隐藏掉。根因:
Renderer._transformEntity字段在引擎层既是缓存又承担 listener owner 角色(add listener on_updateFlagManager)。Entity._remap走CloneManager.cloneProperty的第 1 步(最高优先级),直接target._transformEntity = remappedEntity,不会经过_setTransformEntity()setter。_cloneTo → _applySkin → _setTransformEntity(rootBone)时,setter 里的相等守卫if (entity !== preEntity)命中(两边都是 remapped rootBone)→ 跳过 add listener。_onTransformChanged监听器从未注册到 rootBone 的_updateFlagManager,rootBone 移动时 SMR.bounds 不会失效,culling 永远用初始 bounds。migration-agent 当前 workaround(compat-runtime.ts,所有 18 个项目都装了):
根治建议:见 §5.1。这本质上是引擎对"哪些字段必须经过 setter"缺少强制约束,需要在 clone 阶段对此类字段提供 hook。
Commit ref:
ef35d1768fix(compat): patch _setTransformEntity to fix SMR bounds frozen after clone3.2
CloneUtils._getEntityHierarchyPath对已 destroy entity 不容错症状:clone prefab 时抛
TypeError: Cannot read properties of undefined (reading 'parent')。根因:
但当
searchEntity本身是undefined(业务 listener 持有 destroyed entity,引擎 nullify 了.entity字段)时,第一行searchEntity.parent就抛 NPE。PR #2992 没有处理这一路径。
migration-agent 当前 workaround:在 converter 层把
_LISTENER_HOLDER_FIELDS显式列出(如clickEvents/pageEvents/scrollEvents等),对应字段在序列化期就剔除掉 destroyed entity 引用。Commit ref:
eeab33b01fix(converters): _LISTENER_HOLDER_FIELDS 补 pageEvents / scrollEvents根治建议:见 §5.2。
3.3
EventHandler[]数据数组事件 clone 后累加症状:HanBao
upgradeInfo.lv点击一次升 16 级;clone 出的 UI 节点点击事件被触发 N 次。根因:cocos 的
Button.clickEvents: ComponentEventHandler[]是数据数组(plain object 数组,每项{ target, _componentName, _handler, customEventData })。Galacean 把它翻译为Signal/NodeEvent.on(...)时,clone 之后业务代码会做events.push(eh)累加,结果一个按钮挂了多份 listener。PR #2992 让 plain object 自动深克隆 + remap entity 引用,但数组事件的"清空 + 重建"语义不是 CloneManager 能感知的——这需要业务层(或 compat 层)按 cocos 语义重写。
migration-agent 当前 workaround(feedback_cocos_data_array_event_compat.md):compat 层 patch
Button.prototype.clickEventsgetter/setter,业务保持 cocos 写法length=0; push(eh)直翻。这不是 CloneManager 能修的,但 RFC 需要把它列出来,说明"clone 自动深拷贝"对数据数组事件模型仍然不够,业务需要识别这一模式。
Commit ref:
feedback_cocos_data_array_event_compat.md3.4 共享
AnimatorController.layers[].stateMachine.states(不是 clone bug,是设计 bug)症状:prefab A 的 Animator setState speed = 2,prefab B 的同 controller 也变 2。
根因:Galacean
AnimatorController是一个 engine 资源,多 prefab 实例共享。state.speed不在 per-instance Animator 上,而在 sharedAnimatorState对象上。clone Animator 时controller走 Assignment(资源共享是对的),但有些状态本来就该 per-instance,引擎 API 不分。migration-agent 当前 workaround:compat
AnimatorStateCompat在 per-Animator pending state speed map 里维护,setter 不写共享对象。这不是 CloneManager 的责任,但是和 clone 语义紧密相关:"哪些是资源(assignment)、哪些是 per-instance state"在 cocos 心智模型里是 component 字段 vs
AnimationClip资源的清晰区分;Galacean 把 state 折到 controller 里就模糊了这条线。Commit ref:
galacean_migration_outputs/*/converters/assets/compat/animation/animator-compat.ts:53-723.5
CloneMode.Shallow几乎是死代码现状:枚举里有
Ignore / Assignment / Shallow / Deep四个;cloneProperty内部把 Shallow 当 Deep 处理(switch 分支不区分);类型推断_inferCloneMode也不会返回 Shallow。问题:Shallow 是"创建新容器、但元素走 Assignment"的语义,理论上对业务自定义类的数组成员有用(每个 entity 独立 array,但 element ref 跨树共享)。当前实现没体现这点,反而是 Deep 在用
cloneMode透传到 array element 时模糊地兼用了。PR #2992 的
_inferCloneMode里只用 Deep / Assignment 两个值返回。建议:要么明确 Shallow 的精确语义("array 元素 1 级浅复制,不进 sub-cloneProperty"),要么 deprecate 并文档化等价于 Deep。当前模糊状态会让用户写
@shallowClone时无法预测行为。3.6
copyFrom调用没有 fall-back 守卫现状:
问题:如果
target不是 source 的同类(比如 source 是Vector3、target 是用户的MyVec仅恰好有同名copyFrom方法),会调到错的实现。PR #2992 的 §2.1 引入了
targetProperty.constructor === sourceProperty.constructor检查,但只在"装饰过/未装饰路径"上判,default branch 内部走copyFrom时不再验类型——因为前面已经走过 inferCloneMode 推断了 deep。但如果用户用@deepClone装饰一个字段,target 里又被另一个 constructor 提前赋了一个"有同名 copyFrom 但语义不同"的对象,这个 case 仍然走 copyFrom 而不报错。建议:default branch 内部加 constructor 校验,不匹配时退回到 "new + per-key" 路径。
3.7
Component._remap用_components.indexOf在多实例场景脆弱问题:同 entity 上挂两个相同类的 Component(cocos 允许),引用某个具体实例时
indexOf依赖插入顺序——但 cloneEngine 当前_createCloneEntity用componentConstructors数组 new,理论上顺序一致,可一旦组件被 sort/swap 过(例如 UI sortIndex 重排),就会 remap 到错的兄弟组件。建议:增加
_componentInstanceIndex(per-entity per-type 的 index)作为更稳的 ID。或者在_createCloneEntity阶段冻结顺序约定并文档化。PR #2992 没碰这一块。
3.8 GLB 内部子节点跨 clone 边界的 entityPath(converter 侧问题,非引擎 clone)
为什么提:用户视角 "clone 出问题" 经常包含 prefab 实例化 GLB 后子节点引用错位,但这一类不是 CloneManager 的问题,是 scene/prefab 序列化层的 entityPath 解析问题。RFC 划清界限:
entity.clone()时 entity tree 内 ref remap。cc.PrefabInstance.propertyOverrides翻成 Galaceanmodifications,并保证跨 nested-prefab / FBX-wrapper / GLB-collapsed 的 entityPath 正确。历史上 D.2~D.5 一系列 commit(
9998a8975/b056edd78/cfe36b2d0/d48d70016等共 ~15 个)都在 converter 侧解决,不影响 PR #2992。RFC 建议:在 CloneManager 文档里加一句"clone 不负责 GLB 内部 entity 的层级解析,那是资产加载器的事",避免后续接入方混淆。
4. 设计原则
收敛上述问题,我提出 4 条原则作为后续 clone 行为的判断依据:
P1. 引用域优先于装饰器
字段的 clone 行为应该由**该引用指向的"对象的所属域"**决定,而不是用户记不记得加
@xxxClone:@ignoreClone退出@deepClone强制独立@assignmentClone退出@deepClone强制(极少用)PR #2992 的
_inferCloneMode实质上已经在按这个方向走,但还缺**"外部句柄"的自动识别**——目前都靠用户写@ignoreClone。这条线建议保持显式(自动识别 DOM 类型容易误伤),但文档要明确"外部句柄必须 @ignoreClone"。P2. setter 副作用必须显式走 hook,不能依赖 setter 守卫
Renderer._setTransformEntity、UICanvas._setComponentMask等"setter 里有重要副作用(add/remove listener)"的字段,不能假设 clone 走的是赋值路径。当前_remap直接覆写_transformEntity就绕过了 setter;如果未来还有别的字段也是这种结构,会再次踩同样的坑。根治方案:
@remapClone(新装饰器)的字段,走_setXxxFromClone钩子(默认就是 setter)。_setTransformEntity(_transformEntity)重置 listener。_transformEntity分离出来到独立字段,避免和 cache 字段耦合。我倾向选项 B(最小侵入,对所有 Renderer 派生类生效)。RFC 应该选定一个并写进引擎。
P3. Map/Set/Array 元素 remap 已对齐 — 但对象 key 的 remap 仍有边界
PR #2992 对
Map.key也走_remap,但 cocos 里几乎不会用 Entity 当 key(用作 value 居多);如果 key 是 plain object,旧逻辑就直接共享 key 引用(PR 没有把 Map.key 走 deep)。建议:明确文档"Map key 不做 deep clone,永远是 reference + 可选 remap";测试用例补一条 "key 是 plain object 的 Map clone 后 key 仍共享" 来锁定行为。
P4. 类型推断的兜底必须保守
_inferCloneMode最后一条return CloneMode.Assignment(未识别的 class 实例)是一个保守兜底。但如果未来有用户写了自定义class Foo { x = new Vector3() }并赋给字段,因为Foo没有copyFrom、constructor !== Object,会走 Assignment——结果 clone 出来 Foo 实例被共享,内部 Vector3 也被共享。建议:要么文档化 "自定义 class 字段默认共享,需要独立请加
@deepClone",要么把推断改为"自定义 class(非引擎资源 / 非外部句柄)默认 Deep"。后者更安全但有兼容风险。PR #2992 选了前者(兜底 Assignment)。我认可这个选择,但需要在 IClone 文档里高亮"自定义 class 默认共享"。
5. 具体待落地动作
按优先级:
5.1 [P0] 修复
_setTransformEntitylistener 注册(§3.1)Renderer._setTransformEntity去掉相等守卫 — 这正是 migration-agent compat 层做的事,引擎合并后 18 个项目可以删 polyfill。protected _setTransformEntity(entity: Entity): void { const preEntity = this._transformEntity; - if (entity !== preEntity) { - preEntity?._updateFlagManager.removeListener(this._onTransformChanged); - entity?._updateFlagManager.addListener(this._onTransformChanged); - this._transformEntity = entity; - } + preEntity?._updateFlagManager.removeListener(this._onTransformChanged); + entity?._updateFlagManager.addListener(this._onTransformChanged); + this._transformEntity = entity; }调用方影响:所有非 clone 路径调用
_setTransformEntity(samEntity)(罕见)会多一次 remove+add,开销 < 1μs,可忽略。或 在 clone 完成后强制刷一次(选项 B):
侵入更小但需要遍历组件树。我倾向第一种。
5.2 [P0]
CloneUtils._getEntityHierarchyPath对 undefined 容错(§3.2)private static _getEntityHierarchyPath(rootEntity, searchEntity, inversePath): boolean { inversePath.length = 0; + if (!searchEntity) return false; // destroyed entity ref while (searchEntity !== rootEntity) { const parent = searchEntity.parent; if (!parent) return false; inversePath.push(searchEntity.siblingIndex); searchEntity = parent; } return true; }外面调用方拿到
false后 fall-back 到"返回原 component / 原 entity"(即外部引用语义),不会塞 undefined 进 clone target。5.3 [P1]
_inferCloneMode加 PR #2992 自身仍欠的 cycle 前置检查日志PR 在 default branch 加了
deepInstanceMap.has(sourceProperty) → 返回缓存。但装饰过@deepClone的同一字段,如果两个 entity 引用同一 source,也会走到这里——目前是直接返回缓存(正确);但如果用户的本意是"每个引用各自一个独立 clone"(罕见),当前 API 无法表达。建议:文档化 "deepClone 装饰的字段,跨字段共享同一 source 引用时,clone 后也共享"(即 cycle dedup 行为是默认且无法关闭)。这是 PR #2992 的设计选择,明文写进 JSDoc。
5.4 [P1] 明确 / Deprecate
CloneMode.Shallow(§3.5)PR #2992 没动 Shallow。两个方案选一:
CloneMode.Shallowdeprecated,提示 "use Deep + 装饰元素类型" 替代;在引擎里走和 Deep 一致的处理。我倾向 B(Shallow 当前没有用户)。
5.5 [P1] 文档化"自定义 class 字段默认 Assignment"(§4 P4)
在
@galacean/engine文档站的 Clone 章节加一节"自定义类型字段",明确推断规则,给出最佳实践:5.6 [P2]
Component._remap加多组件场景测试(§3.7)补一个测试:同 entity 挂两个
Behavior实例,prefab clone 后引用各自的 instance,验证_components.indexOf在简单情况下能区分。如果失败,加_componentInstanceIndex。5.7 [P2] 测试覆盖补充
PR #2992 当前测试已覆盖:
缺:
Vector3field 不被 source 覆写(这是 PR §2.1 的核心 case,目前测试里没单独验证)@assignmentClone显式标记字段 + target 已有同类型实例 → 仍然 Assignment(PR §2.4 的核心 case)5.8 [P3] 文档"clone vs scene/prefab 序列化"边界(§3.8)
在 CloneManager JSDoc 顶部加:"CloneManager handles runtime
entity.clone()only. Cross-asset references (GLB sub-entity refs, nested PrefabResource overrides) are resolved at asset load / scene deserialization, see PrefabResource / SceneLoader."6. 行为对照表(最终态)
按字段类型 × 装饰器组合的预期行为:
@ignoreClone@assignmentClone@deepClone{}_remapfall-back: 返回原 ref7. 破坏性变更评估
PR #2992 已经引入了一处破坏:
测试
CloneUtils.test.ts:264-269显示了行为变化:影响范围:所有 cocos→galacean 业务代码里"私有缓存对象"在旧版会共享,clone 后两个实例改 cache 会互相影响——这恰恰是 cocos 业务的 bug。修复后正确。
风险:原生 galacean 用户如果有意在 clone 后共享 plain object,会受影响。需在 release note 标注,建议加
@assignmentClone显式保留旧行为。§5 提议的其他改动(5.1 setter listener、5.2 NPE 容错)都是 bug fix,无 API 破坏。
8. 不在本 RFC 范围
为避免 scope creep,以下问题虽然和 clone 沾边,不在本 RFC 处理:
.clone()滥用(fact-card 已修,migration-agent 内问题)。getMaterial()vsgetInstanceMaterial()的 clone-on-write 语义 — 已对齐,但属于 Renderer API 设计。9. 决策待定项
需要 luzhuang / 引擎 owner 拍板的:
_setTransformEntity修复路径:去守卫 vs clone 后强制刷新 vs 拆字段,选哪个?CloneMode.Shallow:实装真实语义 vs deprecate?10. 落地路线图(如果方案通过)
_setTransformEntity修复 +_getEntityHierarchyPathNPE 容错,单独 PR。CloneMode.Shallow决策。installSetTransformEntityPolyfill,验证 18 个项目无 regression。附录 A:migration-agent 项目里 clone 相关问题完整索引
引用源 commit hash 见 git log;下表只列代表性 case:
ef35d1768+ compat-runtime.ts:677installSetTransformEntityPolyfilleeab33b01_LISTENER_HOLDER_FIELDSnew Vector3()被共享.clone()切断引用b626b55f1+7fd9cb805@ignoreClone× 11 处附录 B:测试用例完整建议清单
讨论:评论于 galacean/engine#2992 或单独开 issue 关联本 RFC。